Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Align casting methods with Python behaviour #497

Merged
merged 15 commits into from
Dec 14, 2022
Merged

Align casting methods with Python behaviour #497

merged 15 commits into from
Dec 14, 2022

Conversation

honno
Copy link
Member

@honno honno commented Oct 18, 2022 •
edited
Loading

Resolves #486 (adding x.__complex__()).

Per interest in having the builtin casting methods (__bool__/__int__/__float__/__complex__/__index__) support arrays of a different dtype kind, this PR also changes the language to clarify that adopters can support such casting. EDIT: now aligning casting methods with Python behaviour generally

(I opted against requiring cross-kind casting in this PR—see #486 (comment)) See #486 (comment)

@honno honno mentioned this pull request Oct 18, 2022
Closed
@honno honno changed the title Add x.__complex__(), unrestrict cross-kind casting in respective array methods Align casting methods with Python behaviour Nov 2, 2022
@honno
Copy link
Member Author

honno commented Nov 2, 2022 •
edited
Loading

Per what we decided upon in our last array meeting (see #486 (comment)), I've updated this PR with my take on how to we align the spec's casting methods with Python's behaviour.

I have some thorough testing for how Python's casting works in https://gist.github.com/honno/98371048b4d5b9fc4a003377a714e827, which I'll have to tidy up (will be useful anyway for the test suite).

As @seberg noted, NumPy seems to 100% follow Python casting behaviour for its arrays (if we're not including a small divergence in behaviour for NumPy scalars). Only notable thing maybe is NumPy not aligning with Python's __index__ (not actually casting I suppose), in that it raises TypeError for bool arrays, where Python bool objects just returns itself.

Not 100% on language used, happy to hear what folk think. Will sleep on it anyway.

honno
honno commented Nov 2, 2022
View reviewed changes
Copy link
Member Author

@honno honno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realised I should probably special case non-finite values for __bool__.

spec/API_specification/array_api/array_object.py Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
@honno
Copy link
Member Author

honno commented Nov 14, 2022

Think this is ready @seberg @asmeurer? With the follow up of specifying <bool-array>.__index__() behaviour as erroneous (context #497 (comment)).

kgryte
kgryte requested changes Nov 14, 2022
View reviewed changes
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Outdated Show resolved Hide resolved
spec/API_specification/array_api/array_object.py Show resolved Hide resolved
@honno
Copy link
Member Author

honno commented Nov 15, 2022

Note previews are broken because of some work I was doing in my fork for #505, but you can change the URLs to lead with .../0/build/draft/index.html like so still.

@honno honno requested a review from kgryte November 17, 2022 11:05
@kgryte kgryte added this to the v2022 milestone Nov 21, 2022
@kgryte kgryte added the API change Changes to existing functions or objects in the API. label Dec 14, 2022
@kgryte
Copy link
Contributor

kgryte commented Dec 14, 2022

I believe all discussion points have been resolved. I'll go ahead and merge. Any further changes can be addressed in follow-up PRs. Thanks, all!

@kgryte kgryte merged commit adca8ce into data-apis:main Dec 14, 2022
@asmeurer asmeurer mentioned this pull request Dec 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lezcano lezcano lezcano left review comments

@kgryte kgryte kgryte approved these changes

@asmeurer asmeurer asmeurer left review comments

@seberg seberg seberg left review comments

Assignees
No one assigned
Labels
API change Changes to existing functions or objects in the API.
Projects
None yet
Milestone
v2022
Development

Successfully merging this pull request may close these issues.

Add __complex__ to array object
5 participants
@honno @kgryte @seberg @asmeurer @lezcano